Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move to ghactions #13

Merged
merged 47 commits into from
Jan 8, 2021
Merged

Move to ghactions #13

merged 47 commits into from
Jan 8, 2021

Conversation

tjurtsch
Copy link
Contributor

@tjurtsch tjurtsch commented Jan 4, 2021

This submodule implements generic scripts that will be used by CI repos (compilers, misc, prog, eda).

This PR replaced another PR from remote branch #12

Upload resulted in an error when there was already package in this
version in channel
https://github.com/antmicro/litex-conda-misc-1/runs/1616885674?check_suite_focus=true
We need Conda environment set when script is executed.
We use this simple counter condition to determine if
all jobs were queued and completed. Not queued jobs
aren't returned in API call.
Since we are sourcing common.sh we should already be inside
Conda environment with pexpect installed
ghactions API call we're using limits number of jobs
returned in one call. If jobs exceed this number they are
grouped into "pages". Page handling code shall be added
when we exceed this number.
These functions split the output logs into foldable groups
@tjurtsch tjurtsch force-pushed the move-to-ghactions branch 2 times, most recently from 5db1e04 to f462e7b Compare January 7, 2021 09:51
@ajelinski
Copy link
Contributor

LGTM now.

@PiotrZierhoffer PiotrZierhoffer merged commit 6a1e26c into master Jan 8, 2021
@umarcor
Copy link
Member

umarcor commented Jan 11, 2021

I'd say I saw some intermediate state of this PR, where the README was "properly" updated. However, some modifications seem gone, and not present after the merge. For example:

Travis scripts for Litex-Hub/litex-conda-(compilers|eda|misc|prog).

was

Scripts for Litex-Hub/litex-conda-(compilers|eda|misc|prog) Continuous Integration.

in https://github.com/litex-hub/litex-conda-ci/pull/13/files/ce25d236f590d242de5cb62ae9b1f26c5613f40e#diff-63aa9fac641138cdd078a654078ea278bdf7b39c88d4fc0843304351d6bb3272.


Overall, the addition of action.yml without further clarification in the README feels very weird. action.yml defines a "Composite Action" to be used in GitHub Actions Workflows through uses: litex-hub/litex-conda-ci@master. On the other hand, the README says that this repo is to be added as a submodule. Moreover, it seems that the composite action does depend on the submodule being added in a very specific location: https://github.com/litex-hub/litex-conda-ci/blob/master/action.yml#L15-L17. Therefore, I would recommend to convert action.yml into build.sh. There is no added value by using the YAML syntax in this case. It's an unnecessary layer of complexity which introduces a maintenance burden.


I would strongly discourage adding Git for Windows' internal tools to the PATH: https://github.com/litex-hub/litex-conda-ci/blob/master/action.yml#L13. See actions/runner-images#1525. What is the motivation for doing so?

@tjurtsch
Copy link
Contributor Author

Readme clearly needs to be fixed, thank you for pointing that out.
It's also true that this submodule/actions mixture needs to be cleaned up. @ajelinski suggested that we shall drop usage of a submodule and move to full "action" approach instead. It's still not better than a shell script, but it will be cleaner than the current mixed approach. So the litex-conda-ci repo will be only referenced in the workflow as an action. IMHO the only added value will be: no submodule bumping.

@umarcor
Copy link
Member

umarcor commented Jan 15, 2021

@tjurtsch I'm not sure that's the best strategy... Let me explain:

Since GitHub Actions were created, there have been two types: "JavaScript/TypeScript" and "Docker".

JavaScript/TypeScript Actions can be executed on any environment (Ubuntu, Windows or macOS), because all environments include nodejs and someone decided that was the "best" language for scripting. However, those have one very obvious problem: the JS package ecosystem is a mess that fights against any optimal use of files. Unless you want all users of the Action to retrieve hundreds of dependencies, you need to "compile" them. At the same time, GitHub Actions does not support semver filters when specifying Actions, and it can neither use releases. All "releases" need to be a branch in the repo. As a result, you are forced to setting up some CI for handling the releases of your JS Actions, you cannot just tag and push. Alternatively, you need developers to "compile" locally before pushing (so they need nodejs on their workstations/laptops).

Docker Actions are nicer because you can wrap any scripting language (say bash or python) in a container, and you can avoid using a browser language for CI. Unfortunately, Docker Actions are only supported on Ubuntu (neither Windows nor macOS), which is not acceptable in many projects. Moreover, the docker images are fixed in Docker Actions. You cannot let the user pass the image as an argument.

The interesting feature of Docker Actions is that you can call existing containers using that syntax. See, for instance: https://github.com/tmeissner/formal_hw_verification/blob/master/.github/workflows/Test.yml#L41-L45. However, you can only call containers which existed before the job was started. That is, you cannot build a container image in one step and use it in the next one using this syntax. You need to use the regular docker run ... image_name ... instead.

Obviously, users complained a lot about the hard to understand limitations, and they recently came up with Composite Actions (the type you used). Those are nice... except they don't allow to compose anything. You cannot call other Actions (use uses:) in a Composite Action. Therefore, it's pretty useless, as you can only wrap scripts with it. The only advantage of this type I could find was, precisely, allowing users to customise the container image used in what was earlier a Docker Action: https://github.com/VUnit/vunit_action/blob/master/action.yml.

As a result, I think you should first evaluate whether you'll be able to describe the logic that you currently use in your scripts, considering the constraints above. If you are already following the submodule approach and that works, maybe you should stick with that. I would suggest the following: rework the existing Composite Action for installing this repo in CI and maybe call it setup-litex-ci. There, just copy the resources of this repo to some known location, or to an arbitrary location and set an envvar. At the same time, remove the submodules and use this Action as the first step in CI. However, use the scripts explicitly in all other steps of CI. That will allow you to extend this repo for providing different entrypoints, without a maintenance burden.

@ajelinski ajelinski deleted the move-to-ghactions branch January 15, 2021 19:55
@ajelinski
Copy link
Contributor

Wow, this is quite an extensive explanation.

TBH realizing how many limitations this Composite Action has was very disappointing for me too. It's hard to understand why other Actions can't be called.

However, in this case I see the biggest value of using such an action in dumping the submodule usage. Bumping those is pretty annoying if you have to do it in four repositories. Using its branches to test any changes also isn't the best experience.

Because litex-hub-ci isn't a third-party Action we could use its master branch in the workflows. The LiteX-Hub maintainers decide when it changes so there's no security risk. Maintainability is easier and when there's a need to test some litex-hub-ci branch we can easily add an option to the workflows to start the build manually with an input specifying what revision of litex-hub-ci is to be used.

You're right that current situation needs to be fixed, because this Action shouldn't use scripts from the repository it runs in. At least there should be $GITHUB_ACTION_PATH instead of those paths for it to make sense. I thought that after doing so the .github/scripts submodule could be removed from all litex-conda-* repositories but I've just realized we have two entrypoints indeed. One for building a package and the other for moving the packages to main label if all builds succeeded.

I must say that your proposal with an action simply installing all scripts definitely makes sense and would also enhance the experience of the litex-conda-ci usage. However, calling scripts directly isn't an option because there are simply too many jobs in litex-conda-* workflow to add more steps. The only option would be to convert the existing action.yml to a script and call it although it also adds at least one step to each job – use such setup-litex-ci action and then call the build script.

Maybe there could be two actions in this repository:

  • build-action
  • master-action

It seems to make sense as both actions share some scripts. The problem of accessing two entrypoints without copying the scripts would be solved.

What do you think about such an idea?

@umarcor
Copy link
Member

umarcor commented Jan 15, 2021

@ajelinski, just to make it clear: you are already doing a nice job and all your ideas are sensible. I've been using GitHub Actions workflows for 18 moths, and I currently maintain JS, Docker and Composite actions. I want to help with this background/know-how I have. However, I help handle CI in several orgs, and am not familiar with all the details of the CI infrastructure in SymbiFlow/litex-hub/antmicro (yet). I might be missing relevant constraints, such as what you said about having too many jobs. Therefore, we need to help each other. Can you please provide a reference of the four repositories you mentioned, so I can have a look and get a better picture? I can guess which repos you mean, but I prefer to be given a specific scope to focus on. Maybe we can rethink some of those steps, in coherence with reshaping this repo.

@ajelinski
Copy link
Contributor

Sorry for the delayed answer @umarcor. Everything is clear, I hope your help will make litex-conda-* CIs even better.

The four repositories that I use litex-conda-* name for are:

They all use litex-conda-ci scripts in their CIs. There's indeed the inconsistency that the action litex-hub/litex-conda-ci@master is called and it uses scripts from the calling repository's submodule. Fortunately it doesn't give any negative effects for the package building right now but we'll definitely need to fix it.

BTW. Using separate jobs in the workflows to build each package instead of using a job with a matrix gives us possibility to properly define intra-repository package dependencies. Unfortunately it results in duplicating a similar job multiple times but it seems inevitable if we want to use needs in such a way. GitHub Actions disallowing usage of the yaml anchors also doesn't help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants